Skip to content

Document named arguments BC policy #14566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2020

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Nov 18, 2020

No description provided.

@carsonbot carsonbot added this to the 4.4 milestone Nov 18, 2020
@derrabus
Copy link
Member

We should attach that footnote to Create a new instance as well because that is a constructor call that could also be done with named arguments.

@wouterj
Copy link
Member Author

wouterj commented Nov 18, 2020

For reference: It's maybe good to include "unless specified otherwise". For instance, it might make sense to support named arguments in attributes or specific data objects (I'm thinking about some of the newer "configuration" data objects in the RateLimiter and Mailer components).

However, before I want to document that, I think we should have a standardized (in Symfony) way of indicating this in the PHPdoc or the like (documentation?).

@nicolas-grekas
Copy link
Member

before I want to document that, I think we should have a standardized (in Symfony) way of indicating this in the PHPdoc or the like (documentation?).

I agree. A PHP8 attribute would be nice. The only issue is where to put it. Maybe we don't need a package for that? After all, that's not mandatory: attributes can be added/read without any implementation. We'd just need to settle on its name (and we could provide an optional package of course).

@derrabus
Copy link
Member

We could build a new contracts package for attributes like that, e.g. compatibility-contracts. Naming is hard, so ideas for a better name are welcome. 🙃

@wouterj
Copy link
Member Author

wouterj commented Nov 18, 2020

I'm not exactly sure what this package should do. Do we want to trigger notices when using an uncovered named argument, like we do when extending a class that's @final?

@derrabus
Copy link
Member

Static code analyzers (phpstan, psalm, PhpStorm) could pick this up and issue a warning. I don't think that we can trigger notices in such a case, can we?

@wouterj
Copy link
Member Author

wouterj commented Nov 18, 2020

I think it would be better to not jump the line and introduce attributes for documentation purposes, before there is a common direction that the tools are moving into. At the moment I can't find anything about attribute support for PhpDocumentor and PHPstan. Psalm seems to lean against attributes for static analysis (but has support for people wanting to use it like that), while PHPstorm added attributes for static analysis.

I think - if we can't do any analysis on it ourselves - the best option is to just add it to the PHPdoc description and e.g. include a small "tip" or "note" in documentation when named arguments are covered (e.g. when introducing the Route attribute, add a small tip "Hey, these named arguments are backwards compatible").

@nicolas-grekas
Copy link
Member

I definitely think that attributes should be used by static analyzers/IDEs yes.
This attribute would tell them when named args are wrongly used, or must be used, etc.

@dunglas
Copy link
Member

dunglas commented Nov 18, 2020

Named arguments is a major feature of PHP8, and is very pleasant to use.

Shouldn't we progressively move from promoting ordered argument to promote named arguments? We could use the proposed annotation to indicate if the class supports the old (ordered) or the new (named) BC promise. Supporting both doesn't worth it in my opinion (too much work).

@javiereguiluz
Copy link
Member

Thank you Wouter.

@javiereguiluz javiereguiluz merged commit a3c287a into symfony:4.4 Nov 20, 2020
@wouterj wouterj deleted the bc-named-arguments branch November 20, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants